-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite simple_token_authentication to a Devise strategy #69
Rewrite simple_token_authentication to a Devise strategy #69
Conversation
Setting up authentication key is no so trivial (the code was failing here). Moreover, currently there is not way to customize this in simple_token_authentication. This can be added as a separate feature, but should not increase the complexity while rewriting to Devise strategy.
After delving into the Devise source code itself, I ended up trying to take this into a slightly different direction to make it more "devise-like." While I haven't gotten it working fully yet, I can push what I have so far if you'd like to take a look. |
See changes in 1329f30 |
This avoids calling [] on nil.
@adamniedzielski If necessary, could you please cherry-pick the @jbender commit(s) in order to stay in sync? |
IMO, making the strategy as devise-like as possible is full of sense. Since the change will be disruptive, I opened a v2.0.0 milestone for this feature. Once that done, while care should be taken to ensure the features of the v1.x series are provided by the strategy, there is no need to care about changing the way the gem must be installed or used and Devise-likeliness is very welcome. |
… naming conventions
OK, so these are all the commits to make my application work after the rewrite. The changes which were necessary on my app's side: class User < ActiveRecord::Base
- acts_as_token_authenticatable
devise :database_authenticatable, :registerable,
- :recoverable, :rememberable, :trackable, :validatable
+ :recoverable, :rememberable, :trackable, :validatable,
+ :simple_token_authenticatable class UsersController < ApplicationController
- acts_as_token_authentication_handler_for User
+ before_action :authenticate_user! Can you please check whether it works for you? |
This is great, guys. Good job all! |
@gonzalo-bulnes We should try to preserve all the features of v1.x series. However, there are some things which might work differently: a) I'm not quite sure how multiple authentication strategies are handled. The logic behind this should be somewhere in Warden, but its code is difficult for me to read. As far as I can tell, at the beginning strategies are selected based on their I am not sure what the ordering of strategies is. I will explore it further. b) multiple # Several token authenticatable models can be handled by the same controller.
# If so, for all of them except the last, the fallback_to_devise should be disabled.
#
# Please do notice that the order of declaration defines the order of precedence.
#
# acts_as_token_authentication_handler_for Admin, fallback_to_devise: false
# acts_as_token_authentication_handler_for SpecialUser, fallback_to_devise: false
# acts_as_token_authentication_handler_for User # the last fallback is up to you This can be difficult to preserve too. I found similar functionality in Devise - https://github.com/plataformatec/devise/blob/master/lib/devise/controllers/helpers.rb#L75 I think we should aim for basic backwards compatibility even in v2.0. We can create aliases for I don't know how to work together on this PR. I see two possibilities: |
Idea of multiple scopes for me was to separate actions between different scopes with acts_as_token_authentication_handler_for User, only: [:new, :edit]
acts_as_token_authentication_handler_for Admin, only: [:delete] Code to achieve this logic with devise: before_action :authenticate_user!, only: [:new, :edit]
before_action :authenticate_admin!, only: [:delete] Logic documented in README is a side-effect of two features: before_action :current_admin
before_action :current_special_user
before_action :authenticate_user! |
@donbobka - thanks for your input! I agree that: acts_as_token_authentication_handler_for Admin, fallback_to_devise: false
acts_as_token_authentication_handler_for SpecialUser, fallback_to_devise: false
acts_as_token_authentication_handler_for User # the last fallback is up to you is strange and I cannot see any real use-case for it. However, it's good to know that it would be possible to achieve after the rewrite. |
Hi @adamniedzielski, When it became possible, I documented how an unique controller could handle token authentication for several models because of #39. But its definitely a rare use case, and I would consider it myself as a bad design smell; that consideration goes beyond the scope of the gem however, and that's why I documented it anyway. The Concerning the PR organization, which one do you prefer, a) or b)? |
With regards to
|
@jbender - you are right, that's the way to limit available strategies. However, this does not solve the problem with existing sessions. If you look at: In such a case If we describe all this configuration in README @gonzalo-bulnes - PR organization - b) should work better |
Here is the branch: spike-refactor-concerns-to-devise-strategy @adamniedzielski |
Thanks @gonzalo-bulnes ! So basically, everybody can now make PR against I need your (community) help! There is still a lot of things to do - especially updating the docs and tests. |
Thank to you @adamniedzielski! |
WIP on #57